Remove mutex from DataStoreService#107
Conversation
lzxddz
force-pushed
the
remove-mutex
branch
from
363e8bd to
7857742
Compare
lzxddz
force-pushed
the
remove-mutex
branch
from
7857742 to
1aaa871
Compare
|
Caution Review failedThe pull request is closed. WalkthroughRemoved per-shard DataStore creation, Initialize/StartDB, and per-shard mappings from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Server
participant ha_eloq
participant DSS as data_store_service_
rect rgba(230,250,255,0.6)
note over ha_eloq,DSS: Startup
Server->>ha_eloq: Initialize()
alt bootstrap or single-node
ha_eloq->>DSS: StartService(true)
else normal
ha_eloq->>DSS: StartService(false)
end
DSS-->>ha_eloq: Service ready
ha_eloq-->>Server: Initialization complete
end
rect rgba(255,240,230,0.6)
note over ha_eloq,DSS: Shutdown
Server->>ha_eloq: eloq_done_func()
ha_eloq->>ha_eloq: reset data_store_service_ (no DisconnectDataStore)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
storage/eloq/ha_eloq.cc(1 hunks)storage/eloq/store_handler(1 hunks)storage/eloq/tx_service(1 hunks)
🔇 Additional comments (2)
storage/eloq/tx_service (1)
1-1: Manual verification required for storage/eloq/tx_service submodule updateI couldn’t fetch the diff between commits 2bd068afeac7e7621c15bd7ac718f6e96cd5ce73 → 4c571f85afe887d79f40d259efaa3601083a9ab2 locally; please review the changes in storage/eloq/tx_service to ensure:
- Removed mutex logic doesn’t introduce new concurrency issues
- Submodule API remains compatible with the simplified DataStoreService::StartService call in ha_eloq.cc
- Any breaking changes in tx_service are properly handled in the main codebase
storage/eloq/store_handler (1)
1-1: Verify submodule commit details manually.
Local inspection of commit57d1ffcb10bac1517b68be745d6fe6da0b757f92failed due to missing history; ensure this submodule update implements the mutex removal in DataStoreService and supports the newdata_store_service_->StartService()pattern.
| // setup local data store service, the data store service will start | ||
| // data store if needed. | ||
| bool ret= | ||
| data_store_service_->StartService((opt_bootstrap || is_single_node)); | ||
| if (!ret) |
There was a problem hiding this comment.
is_single_node is always true, so clustered nodes now take the single-node start path.
is_single_node is initialized to true above and never flipped to false when we fetch topology from a peer. After this change the expression (opt_bootstrap || is_single_node) therefore always evaluates to true, so every node (including multi-node deployments) now forces the single-node StartService path. That regresses clustered setups that previously relied on the service staying in follower mode. Please compute the actual single-node state (e.g., from the loaded ds_config) before invoking StartService, or pass the existing flag that already encodes it, so clustered nodes don’t wrongly bootstrap themselves.
🤖 Prompt for AI Agents
In storage/eloq/ha_eloq.cc around lines 2705-2709, is_single_node is initialized
to true earlier and never updated, so the expression (opt_bootstrap ||
is_single_node) always evaluates to true and forces single-node StartService;
update the code to compute the real single-node state before calling
StartService by deriving it from the loaded ds_config (or use the existing flag
that already encodes cluster membership) — e.g., set is_single_node = (ds_config
indicates no peers / cluster size == 1) or use the provided cluster flag, then
call data_store_service_->StartService((opt_bootstrap || is_single_node)) so
only genuine single-node deployments take the single-node path.
2 participants
Summary by CodeRabbit
Refactor
Chores